Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PL-6 Generalize the Search API fields #39

Merged
merged 13 commits into from
Sep 19, 2018
Merged

PL-6 Generalize the Search API fields #39

merged 13 commits into from
Sep 19, 2018
+14 −414

Conversation

Kbentham
Copy link
Contributor

@Kbentham Kbentham commented Aug 6, 2018 •
edited
Loading

https://palantir.atlassian.net/browse/PL-6

This module splits out the Search API Fields into a new module search_api_field_map. Can be tested using the demo site we have set up. See PL-6-generalize-search-api-fields for more information.

@Kbentham Kbentham mentioned this pull request Aug 6, 2018
Merged
2 tasks
froboy
froboy suggested changes Aug 13, 2018
View reviewed changes
Copy link
Contributor

@froboy froboy left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As suggested on the ticket, this should include two modules: Search API Mapped Field (search_api_mapped_field) and Search API Mapped Terms (search_api_mapped_terms). These will eventually go into a new repo entitled Search API Mapped (search_api_mapped) (not sure about this name). The new module should include a suggestion that it's recommended to use in tandem with search_api_federated_solr.

@Kbentham please move the submodule to the new repo, https://github.com/palantirnet/search_api_field_map and ignore the above about splitting fields and terms. Just keep it all together.

modules/search_api_fields/config/install/image.style.search_api_fields_image.yml Outdated
@@ -0,0 +1,13 @@
langcode: en
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't directly related to the fields implementation so it should stay in search_api_federated_solr.

@froboy
Copy link
Contributor

froboy commented Aug 27, 2018

I think https://github.com/palantirnet/search_api_field_map should be added as a dependency to this module.

froboy
froboy reviewed Aug 29, 2018
View reviewed changes
src/Plugin/search_api/processor/FederatedFields.php Outdated
@@ -4,13 +4,13 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kbentham Why isn't this file removed? Isn't https://github.com/palantirnet/search_api_field_map/blob/master/src/Plugin/search_api/processor/MappedFields.php the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the same I have removed the file.

@froboy
Copy link
Contributor

froboy commented Aug 29, 2018 •
edited
Loading

@Kbentham in testing I noticed that some of the fields (title, image) aren't getting rendered on the page here. It looks like https://github.com/palantirnet/search_api_federated_solr/blob/master/search_api_federated_solr.module#L76 needs to be updated with the new field names. That should... i think... still stay in this module though, as it's necessary for the react things, but not necessarily other uses cases.

Current data in solr (on this PR):

"sm_federated_image": [
  "http://federated-search-demo.local/sites/default/files/styles/search_api_federated_solr_image/public/mushrooms-umami.jpg?itok=2-j6RNl0"
],
"sm_federated_title": [
  "The umami guide to our favorite mushrooms"
],

Desired data (from #40)

"ss_federated_image": "http://federated-search-demo.local/sites/default/files/styles/search_api_federated_solr_image/public/mushrooms-umami.jpg?itok=YKyunezh",
"ss_federated_title": "The umami guide to our favorite mushrooms",

@Kbentham
Copy link
Contributor Author

Kbentham commented Sep 4, 2018

@froboy I fixed the issues you found.

@froboy
Copy link
Contributor

froboy commented Sep 19, 2018

@Kbentham I generalized that statement, and it should work for other fields if they exist now. Thanks for your work, this looks good now.

@froboy froboy merged commit a04466b into master Sep 19, 2018
@froboy froboy deleted the PL-6-generalize-search-api-fields branch September 19, 2018 19:18
@Kbentham Kbentham mentioned this pull request Sep 19, 2018
Merged
byrond pushed a commit that referenced this pull request May 24, 2019
@jesconstantine @byrond
add #39 to 7.x-2.x umhs-28: use canonical URL for results
f7fbf25
@byrond byrond mentioned this pull request May 24, 2019
Merged
jesconstantine added a commit that referenced this pull request May 29, 2019
@jesconstantine
add #39 to 7.x-2.x umhs-28: use canonical URL for results (#94)
963c40f 
add #39 to 7.x-2.x umhs-28: use canonical URL for results
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@froboy froboy froboy approved these changes

Assignees
No one assigned
Labels
Ready For Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Kbentham @froboy